Add configurable envars under mdc logging#1183
Conversation
| @@ -0,0 +1,5 @@ | |||
| [logging] | |||
There was a problem hiding this comment.
I'm not sure what's the appropriate default values here, correct it if needed pls.
| result.put( BUILD_COMMIT_KEY, getOpenshiftBuildCommit() ); | ||
| result.put( BUILD_NAME_KEY, getOpenshiftBuildName() ); | ||
| result.put( BUILD_NAMESPACE_KEY, getOpenshiftBuildNamespace() ); | ||
| return result; |
There was a problem hiding this comment.
I would suggest we don't config those. It will be a burden before deploying a new instance. And, I wonder whether we can get it before building the image? i.e., how to get openshift buildname/commit before we make the build? I guess the config doesn't know the info. I might miss something but this sounds strange to me.
There was a problem hiding this comment.
I will also confirm under NOS-1658, thanks.
There was a problem hiding this comment.
I'd agree with this, but just to the extent that we shouldn't have named methods for those envars at all. In fact, we shouldn't have getter methods for any of these envars...that will limit how much flexibility we have to tailor the list of envars in the future. See my comment above for how I'd like this configuration to work...it will even accommodate aliasing, so we can respond to future OpenShift changes.
|
|
||
| public void putEnvironment() | ||
| { | ||
| objectMapper = new IndyObjectMapper( true ); |
There was a problem hiding this comment.
might just use the above injected one?
There was a problem hiding this comment.
yeah, I will remove in new patch
There was a problem hiding this comment.
My main concern is that LoggingConfig should focus on configuring which environment variable keys to extract, and what names to use when injecting them into the MDC.
We might also rename LoggingConfig (maybe I got the name wrong there) to something like MDCConfig, since we're really tuning what the MDC contains.
Looking more closely at this, I'm concerned that we won't get context information on things that happen as a result of cache listeners, expired content, etc. since they don't start with ResourceManagementFilter. I think we may need to adjust this to work through the layout after all, as was the original plan. If we did that, we could have context info regardless of where the work started. To do this, we could let the logging system set itself up using the class name, then use CDI.getCurrent() to get the injection context for looking up the MDCConfig...or we could push the configuration information directly into the layout config itself, in the XML, using a property that passed an inlined version of these mappings when logback initialized. The format for an XML configuration for this might look like:
<environment-variables>OPENSHIFT_BUILD_NAMESPACE=BUILD_NAMESPACE, HOSTNAME=HOSTNAME</environment-variables>
But I think we have to find a way to make all log messages include this information, so we can tell where they're coming from. If we don't, it will actually increase confusion in the aggregated log, which will be worse than just not having those log messages at all.
| result.put( BUILD_COMMIT_KEY, getOpenshiftBuildCommit() ); | ||
| result.put( BUILD_NAME_KEY, getOpenshiftBuildName() ); | ||
| result.put( BUILD_NAMESPACE_KEY, getOpenshiftBuildNamespace() ); | ||
| return result; |
There was a problem hiding this comment.
I'd agree with this, but just to the extent that we shouldn't have named methods for those envars at all. In fact, we shouldn't have getter methods for any of these envars...that will limit how much flexibility we have to tailor the list of envars in the future. See my comment above for how I'd like this configuration to work...it will even accommodate aliasing, so we can respond to future OpenShift changes.
| HOSTNAME=indy-120-vhxrk | ||
| OPENSHIFT_BUILD_COMMIT=6c17716d024f360164e9461b783da65212fb54b3 | ||
| OPENSHIFT_BUILD_NAME=indy-100 | ||
| OPENSHIFT_BUILD_NAMESPACE=newcastle No newline at end of file |
There was a problem hiding this comment.
See my comment above about format. We should never need to fill in actual values here, the point we're going for is to pull them out of the system environment variables, allowing OpenShift to set them when it spins up a pod.
| { | ||
| try | ||
| { | ||
| MDC.put( ENVIRONMENT, objectMapper.writeValueAsString( loggingConfig.getEnvars() ) ); |
There was a problem hiding this comment.
Could we iterate through the keys in the LoggingConfig and put them directly into the MDC instead of putting them into a new map under it?
BTW, any environment we can't read (isn't declared on the system) should have the value UNKNOWN added, so we can detect them in the log output and use that to fix the problem in the pod / deployment.
There was a problem hiding this comment.
If we are going to send them as a JSON to common logging, to group them in a map would be better. We have other MDC entries like the request-id, etc. If we set the env vars directly, we need to be sure not to override existing keys. We may add new keys in the future and slowly this could be a burden to maintain. But I am not sure how to refer it in logback.xml, like this? %X{evn.hostname} . If we don't need to print it directly to log file, that is no question though.
|
+1 for renaming it to something else. The LoggingConfig makes me think about to control sth about logback.xml. Now I know it is about MDC-to-env key-to-key mapping. how about EnvironmentConfig? @yma88 for the question about the configuration, if you find you will need the parameter() (which is flexible), just remove those annotations on setter methods, or remove setters themselves. Because in this case, the setting is done in the parameter(), your bean only needs getters. |
|
Hi, @jdcasey @ruhan1 thanks very much for your detailed comments, I understand your points and on the way of turning code. |
…ing, add MDC entry by JsonLayout extender
jdcasey
left a comment
jdcasey
left a comment
There was a problem hiding this comment.
Looks good. I like the idea of handling the envar map setup during configuration. At first that was confusing to me, but looking more closely it's clear that it will perform better than pulling those envars each time, and those values should not change during the pod lifecycle.
* Add configurable envars under mdc logging * ADD static SYSTEM_ OPENSHIFT ENV * EnvironmentConfig refactor, make it focusing on variable keys extracting, add MDC entry by JsonLayout extender
* Remove exception stacktrace for remote url Malformed exception I really don't think this exception stack trace is needed as all useful info has been logged, And it occupied bunch of the log space and polluted the log reading. So removed it. * Use same package type for promotion tmp remote repo * Add separator between name and timestamp in promotion tmp remote repo name * Fix issue for temporary remote not deleted after promotion validation * Add system level gauges for metrics * Avoid any exception break metrics reporting in SystemGaugeSet * Add path promotion files count metrics * Fix license headers * Align SNAPSHOT versions * Disable docker for release for now * [maven-release-plugin] prepare release indy-parent-1.7.4 * [maven-release-plugin] prepare for next development iteration * Remove koji ftests dep and align SNAPSHOT * Update weft to 1.14 * [maven-release-plugin] prepare release indy-parent-1.7.5 * [maven-release-plugin] prepare for next development iteration * Add metrics to monitor store disk usage * Use MetricSetProvider for SystemGaugeSet to enable CDI * Use storage directory directly for monitoring * Use storage path in indy configuration instead for storage metrics * Fix test failure * Support jdbc store for folo-sealed ISPN cache * Fix TrackingKey mapper for WrappedByteArray type Add WrappedByteArray type check in TrackingKey2StringMapper which refers ISPN DefaultTwoWayKey2StringMapper implementation. And also adjust some ISPN config for default settings. * Fix way of WrappedByteArray for TrackingKey by using ObjInputStream * Use externalize but not direct obj stream of TrackingKey2StringMapper * Extract abstract class for key2StringMapper with WrappedArrayByte handling * Allow workflow like promotion to bypass readonly flag * Add configurable envars under mdc logging (#1183) (#1194) * Add configurable envars under mdc logging * ADD static SYSTEM_ OPENSHIFT ENV * EnvironmentConfig refactor, make it focusing on variable keys extracting, add MDC entry by JsonLayout extender * fixing serialization / deserialization in tracking record entries (#1195) * Catch all exceptions in promotion executeValidationRule (#1201) * Fix typo of Storertifact.copyBase() * [1.7.x] Refactor CustomJsonLayout, and test it locally (#1200) * Refactor CustomJsonLayout to use default logback layout configuration for envars This will skip the period when CDI isn't started, when it's still trying to log things. It also skips the need for CDI.current() to lookup the environment configuration, putting the envar extraction config directly in logback.xml Conflicts: deployments/launcher/src/main/etc/indy/logging/logback.xml * Remove old CustomJsonLayout class * remove unused constant * Be more careful about empty strings when pulling envars * adjust example kafka config to include SSL params * Generate metadata for a group containing Maven plugins artifacts (#1207) * [1.7.x] Indy promote perf (#1205) * Performance fixes for promotion * Throw timeout exception in runParallelAndWait * Revert default promotion groovy to use paralleledEach * Add skipVersionTest flag when doing Koji path mask repair (#1210) * [1.7.x] Support paralleledInBatch in promotion tool (#1209) * Support paralleledInBatch in promotion tool * Move batchsize to config and fix commented issues * Update license header * [maven-release-plugin] prepare release indy-parent-1.7.6 * [maven-release-plugin] prepare for next development iteration * put tracking id into the MDC (#1214) * [1.7.x] Fix baseUrl (#1217) * Add MDC entries to mark progress in content promotion (#1216) * Add MDC entries to mark progress in content promotion * fix iteration depth MDC injection clear validation tool iteration MDC items,and give a warning when iteration depth is > 0 * [1.7.x] Promotion error consistency and improved by-path lock scoping (#1219) * Make error reporting consistent for promotion results Set a top-level error if validation fails, and add a succeeded() method too. Also, pull the error/validation handling up into the abstract class, and set a constructor so we can call super() * lock the target repo for paths promotion such that validation is included in scope * fix two tests broken by error/succeeded changes in promotion result classes * stop using ThreadContext for parallel iteration depth counting (#1221) * Remove nested paralleledEach from default promotion groovy (#1222) * Fix byPath promotion gauges (#1225) * Add MDC entries to mark progress in content promotion (#1216) * Add MDC entries to mark progress in content promotion * fix iteration depth MDC injection clear validation tool iteration MDC items,and give a warning when iteration depth is > 0 * [1.7.x] Promotion error consistency and improved by-path lock scoping (#1219) * Make error reporting consistent for promotion results Set a top-level error if validation fails, and add a succeeded() method too. Also, pull the error/validation handling up into the abstract class, and set a constructor so we can call super() * lock the target repo for paths promotion such that validation is included in scope * fix two tests broken by error/succeeded changes in promotion result classes * stop using ThreadContext for parallel iteration depth counting (#1221) * Fix license headers * Use storage path in indy configuration instead for storage metrics * Update license header * Remove exception stacktrace for remote url Malformed exception I really don't think this exception stack trace is needed as all useful info has been logged, And it occupied bunch of the log space and polluted the log reading. So removed it. * Fix test failure * Update license header * Allow workflow like promotion to bypass readonly flag
* Remove exception stacktrace for remote url Malformed exception I really don't think this exception stack trace is needed as all useful info has been logged, And it occupied bunch of the log space and polluted the log reading. So removed it. * Use same package type for promotion tmp remote repo * Add separator between name and timestamp in promotion tmp remote repo name * Fix issue for temporary remote not deleted after promotion validation * Add system level gauges for metrics * Avoid any exception break metrics reporting in SystemGaugeSet * Add path promotion files count metrics * Fix license headers * Align SNAPSHOT versions * Disable docker for release for now * [maven-release-plugin] prepare release indy-parent-1.7.4 * [maven-release-plugin] prepare for next development iteration * Remove koji ftests dep and align SNAPSHOT * Update weft to 1.14 * [maven-release-plugin] prepare release indy-parent-1.7.5 * [maven-release-plugin] prepare for next development iteration * Add metrics to monitor store disk usage * Use MetricSetProvider for SystemGaugeSet to enable CDI * Use storage directory directly for monitoring * Use storage path in indy configuration instead for storage metrics * Fix test failure * Support jdbc store for folo-sealed ISPN cache * Fix TrackingKey mapper for WrappedByteArray type Add WrappedByteArray type check in TrackingKey2StringMapper which refers ISPN DefaultTwoWayKey2StringMapper implementation. And also adjust some ISPN config for default settings. * Fix way of WrappedByteArray for TrackingKey by using ObjInputStream * Use externalize but not direct obj stream of TrackingKey2StringMapper * Extract abstract class for key2StringMapper with WrappedArrayByte handling * Allow workflow like promotion to bypass readonly flag * Add configurable envars under mdc logging (Commonjava#1183) (Commonjava#1194) * Add configurable envars under mdc logging * ADD static SYSTEM_ OPENSHIFT ENV * EnvironmentConfig refactor, make it focusing on variable keys extracting, add MDC entry by JsonLayout extender * fixing serialization / deserialization in tracking record entries (Commonjava#1195) * Catch all exceptions in promotion executeValidationRule (Commonjava#1201) * Fix typo of Storertifact.copyBase() * [1.7.x] Refactor CustomJsonLayout, and test it locally (Commonjava#1200) * Refactor CustomJsonLayout to use default logback layout configuration for envars This will skip the period when CDI isn't started, when it's still trying to log things. It also skips the need for CDI.current() to lookup the environment configuration, putting the envar extraction config directly in logback.xml Conflicts: deployments/launcher/src/main/etc/indy/logging/logback.xml * Remove old CustomJsonLayout class * remove unused constant * Be more careful about empty strings when pulling envars * adjust example kafka config to include SSL params * Generate metadata for a group containing Maven plugins artifacts (Commonjava#1207) * [1.7.x] Indy promote perf (Commonjava#1205) * Performance fixes for promotion * Throw timeout exception in runParallelAndWait * Revert default promotion groovy to use paralleledEach * Add skipVersionTest flag when doing Koji path mask repair (Commonjava#1210) * [1.7.x] Support paralleledInBatch in promotion tool (Commonjava#1209) * Support paralleledInBatch in promotion tool * Move batchsize to config and fix commented issues * Update license header * [maven-release-plugin] prepare release indy-parent-1.7.6 * [maven-release-plugin] prepare for next development iteration * put tracking id into the MDC (Commonjava#1214) * [1.7.x] Fix baseUrl (Commonjava#1217) * Add MDC entries to mark progress in content promotion (Commonjava#1216) * Add MDC entries to mark progress in content promotion * fix iteration depth MDC injection clear validation tool iteration MDC items,and give a warning when iteration depth is > 0 * [1.7.x] Promotion error consistency and improved by-path lock scoping (Commonjava#1219) * Make error reporting consistent for promotion results Set a top-level error if validation fails, and add a succeeded() method too. Also, pull the error/validation handling up into the abstract class, and set a constructor so we can call super() * lock the target repo for paths promotion such that validation is included in scope * fix two tests broken by error/succeeded changes in promotion result classes * stop using ThreadContext for parallel iteration depth counting (Commonjava#1221) * Remove nested paralleledEach from default promotion groovy (Commonjava#1222) * Fix byPath promotion gauges (Commonjava#1225) * Add MDC entries to mark progress in content promotion (Commonjava#1216) * Add MDC entries to mark progress in content promotion * fix iteration depth MDC injection clear validation tool iteration MDC items,and give a warning when iteration depth is > 0 * [1.7.x] Promotion error consistency and improved by-path lock scoping (Commonjava#1219) * Make error reporting consistent for promotion results Set a top-level error if validation fails, and add a succeeded() method too. Also, pull the error/validation handling up into the abstract class, and set a constructor so we can call super() * lock the target repo for paths promotion such that validation is included in scope * fix two tests broken by error/succeeded changes in promotion result classes * stop using ThreadContext for parallel iteration depth counting (Commonjava#1221) * Fix license headers * Use storage path in indy configuration instead for storage metrics * Update license header * Remove exception stacktrace for remote url Malformed exception I really don't think this exception stack trace is needed as all useful info has been logged, And it occupied bunch of the log space and polluted the log reading. So removed it. * Fix test failure * Update license header * Allow workflow like promotion to bypass readonly flag
3 participants
This envars worked for Datahub kafka consumer(local) test, message output is like: http://pastebin.test.redhat.com/741859